Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make confirm popup for adult consent #2419

Merged
merged 25 commits into from
Apr 18, 2024
Merged

Make confirm popup for adult consent #2419

merged 25 commits into from
Apr 18, 2024

Conversation

SleeplessOne1917
Copy link
Member

Makes UI for a backend change. This isn't fully complete because there are more things that I need to clarify.

How should the content_warning site setting interact with the enable_nsfw site setting? I assume that content_warning should not be able to have a value when enable_nsfw is off. However, if I understand correctly, having enable_nsfw set to true while content_warning is blank still blurs nsfw images by default, while the blurring is disabled if a content_warning is set. I think it's pretty unintuitive, since the relation between the content warning and post blurring is not clear from just looking at the settings.

One idea I have around this is including a blurb explaining this, though I imagine there are better alternatives and I want to know if anyone thinks of any.

Another thing I noticed is that the user settings for showing nsfw and blurring nsfw are visible even when the site has enable_nsfw set to false. The option for blurring nsfw is also able to be changed independently of showing nsfw, which seems wrong.

Besides the stuff above, what I have left with this is:

  • Add UI to site form
  • Add translations

I'll get those 2 parts done once the other stuff is hashed out.

if (confirm(siteRes.site_view.site.content_warning)) {
localStorage.setItem(adultConsentLocalStorageKey, "true");
} else {
history.back();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I include an alert like lemmy nsfw does?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think so. It doesn't have to be a javascript::alert like theirs, but that would probably have the best compatibility. I'd probably prefer a proper modal tho, so that we can markdown render the content warning.

The most important thing is that the site behind the alert is hidden until they click a let me in button.

@dessalines
Copy link
Member

I assume that content_warning should not be able to have a value when enable_nsfw is off.

Seems correct to me, maybe hide the content_warning field if enable_nsfw = false.

However, if I understand correctly, having enable_nsfw set to true while content_warning is blank still blurs nsfw images by default, while the blurring is disabled if a content_warning is set. I think it's pretty unintuitive, since the relation between the content warning and post blurring is not clear from just looking at the settings.

I suppose content_warning is a rare override, that should disable all blurring and nsfw settings. As long as the nsfw check is extracted to a function that takes these into account, it shouldn't be too difficult.

One idea I have around this is including a blurb explaining this, though I imagine there are better alternatives and I want to know if anyone thinks of any.

I wouldn't worry about that, its up to the server-runners to put that in their content_warning if they wish.

Another thing I noticed is that the user settings for showing nsfw and blurring nsfw are visible even when the site has enable_nsfw set to false. The option for blurring nsfw is also able to be changed independently of showing nsfw, which seems wrong.

That could also be fixed here, if you'd like. We could hide those user settings fields because they're pointless if the site doesn't allow NSFW.

@SleeplessOne1917
Copy link
Member Author

I suppose content_warning is a rare override, that should disable all blurring and nsfw settings. As long as the nsfw check is extracted to a function that takes these into account, it shouldn't be too difficult.

The logic sounds good. The other half of the problem is communicating this to the user in an effective way. While I assume an instance admin should be at least somewhat familiar with the docs, I'd rather try to avoid that with UX design.

One idea I have around this is including a blurb explaining this, though I imagine there are better alternatives and I want to know if anyone thinks of any.

I wouldn't worry about that, its up to the server-runners to put that in their content_warning if they wish.

In this case, I'm not talking about notifying the users about that. I'm talking about making it clear on the site settings UI that admins use.

Everything else you said sounds good and I'll get to work on it.

@SleeplessOne1917 SleeplessOne1917 marked this pull request as ready for review April 14, 2024 14:25
@SleeplessOne1917
Copy link
Member Author

I'm almost done with this issue. The only thing left is making translations. If reviewers are good with the verbiage I picked, I'll make the translations PR.

Here is what the section of the admin settings looks like:
Screenshot_20240414_101655

I recorded some videos of the popup, but github is being dumb and won't let me upload them.

@Nutomic
Copy link
Member

Nutomic commented Apr 15, 2024

How should the content_warning site setting interact with the enable_nsfw site setting?

enable_nsfw is only used in the backend to disable thumbnail generation for nsfw posts. I suppose we could get rid of that value and instead check if content_warning exists to generate nsfw thumbnails. But that would be a breaking change. So for now we can mark enable_nsfw as deprecated.

LemmyNet/lemmy#4627

@SleeplessOne1917
Copy link
Member Author

Since enable_nsfw is going to be deprecated, do you want me to remove the option from the admin settings page? Or should I wait until it's phased out for good?

@Nutomic
Copy link
Member

Nutomic commented Apr 15, 2024

So far the setting is still in use, so it doesnt make sense to remove it yet.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, but IMO the modal should hide (or blur) everything in the background, and might need some re-thinking to make that work.

Even if you set every picture to blur, there could still be some things like large offensive text you wouldn't want anyone going to this site for the first time to see.

The strings you made seem fine to me, so you can i18n them whenever.

@Nutomic
Copy link
Member

Nutomic commented Apr 16, 2024

Im not sure how lemmynsfw.com is doing it, somehow they are hiding the page entirely while the popup is showing. Maybe you can figure it out from their code.

main...lemmynsfw:lemmy-ui:main

@SleeplessOne1917
Copy link
Member Author

I figured out the blur:
image

@dessalines @Nutomic With the blur and translations done, this should be good to merge.

@SleeplessOne1917 SleeplessOne1917 enabled auto-merge (squash) April 16, 2024 13:46
@dessalines
Copy link
Member

Its still showing the site for a second or so:

Screen_Recording_20240416_100259_Kiwi.Browser.mp4

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its faster, but still like 100-200ms for me. Would probably be good to try it another way: let the site render, with a default invisible class, that gets removed after the content warning passes.

{contentWarning && (
<AdultConsentModal contentWarning={contentWarning} />
)}
<AppForwardRef ref={this.rootRef} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just set an invisible class depending on the contentWarning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I handle the blur by setting the background-filter for the top level of the modal element. I've tried doing it by putting a class on app, but that always ended up blurring out the modal as well. Another approach I tried was just having all of the lifecycle methods and event handlers be handled in the original top-level App component. This seemed to work, but would cause the loading skeleton to flash in the blurred background each second the redirect time counts down.

I'm going to see if I can handle this all at the very top level componentDidRoute. I strongly suspect the issue is due to the order rendering is done in. Before my most recent change, the logic for showing the modal didn't run until after the top level App component mounted and rendered. In the current iteration, componentDidMount for the consent modal is being called simultaneously with the componentDidMount for the App component. I think moving this logic to the very root of the component tree as a parent of App should solve the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. If all else fails we might have to just resort to a simple confirm dialog on the SSR like lemmynsfw does it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current iteration, componentDidMount for the consent modal is being called simultaneously with the componentDidMount for the App component.

setState in componentWillMount will affect the first render of the component. It gets called on the server too.

To avoid ever showing the wrongly un/blurred version after a page refresh, you'd probably need a create-ssr-html-hack similar to the auto detected light/dark themes. It's the same problem, the server doesn't know the real style to use, and whatever the server decides to show will otherwise be visible at least until startClient is called.

<script nonce="${cspNonce}">
if (!document.documentElement.hasAttribute("data-bs-theme")) {
const light = window.matchMedia("(prefers-color-scheme: light)").matches;
document.documentElement.setAttribute("data-bs-theme", light ? "light" : "dark");
}
</script>

Copy link
Member Author

@SleeplessOne1917 SleeplessOne1917 Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried my idea and it didn't work. I can attempt @matc-pub's suggestion, although I foresee this will cause me to run into the opposite issue: the page flashing as blurred for a second when it shouldn't be. This is because if content_warning is set, the browser's local storage also has to be checked to see if it needs to ask for consent.

Edit: @matc-pub's suggestion would most likely work if I used a cookie instead of local storage. I'll give that an attempt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried the cookie approach with and without a data attribute. The HTML and CSS that gets returned from the server before client side JS runs still isn't blurred. I think the problem is that bootstrap modals depend on being run in the browser to render. We'll either have to live with a slight delay for when the blur starts or go back to using the regular confirm dialog.

@matc-pub
Copy link
Collaborator

#2430 is a PR to this PR. (Not sure how this will show up)

The PR reverts the commit with the changes in App.tsx and add does apply the blur analog to data-bs-theme attribute.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, since @matc-pub helped with this also, let's have them take a look and approve it also.

BTW @matc-pub , would you like to be a maintainer / collaborator for lemmy-ui? LMK and I'll add you.

id="app"
className="lemmy-site"
ref={this.rootRef}
data-adult-consent={this.isoData.showAdultConsentModal || null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner.

})}
/>
</picture>
!this.isoData.showAdultConsentModal && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested, and it seems to handle the re-rendering fine.

@SleeplessOne1917 SleeplessOne1917 merged commit 643c1f6 into main Apr 18, 2024
2 checks passed
@dessalines
Copy link
Member

Oops I forgot to turn off auto-merge, my bad. Still do a look over and maybe another PR if anything's crucial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants